fix(search): summarize insight when missing name #18809
Conversation
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
|
Size Change: 0 B Total Size: 1.97 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
# Conflicts: # frontend/__snapshots__/components-command-bar--search.png
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
| return extra_fields.name ? ( | ||
| <span>{extra_fields.name}</span> | ||
| ) : ( | ||
| <i> | ||
| {summarizeInsight(extra_fields.query as Node | null, extra_fields.filters as Partial<FilterType>, { | ||
| aggregationLabel, | ||
| cohortsById, | ||
| mathDefinitions, | ||
| })} | ||
| </i> | ||
| ) |
There was a problem hiding this comment.
This doesn't seem right, because there shouldn't be insights that have neither name nor derived_name. Facilitating searchability is a major reason for derived_name being saved, so we shouldn't paper over this. Should we revert this and fix the root cause, or am I missing something here? @thmsobrmlr
There was a problem hiding this comment.
Hmm, I've reimplemented what we do for saved insights as derived_name isn't set for insights in production. Happy to look into it, but would appreciate pointers on how/where we set it. And wouldn't revert this change for now, as we do the same thing in other places and it works for now.
There was a problem hiding this comment.
Digging a bit it seems that we've at one point set the derived_name frontend side when saving an insight, but don't do so now. We obviously can't get the names back this way (and this wouldn't work for insights generated via API), so I'll probably have to duplicate the javascript function in python.
Problem
Insights without a name show up empty in production.
Changes
This PR uses the
summarizeInsightfunction instead ofderived_nameto display an alternative display name for insights.How did you test this code?
Tested locally